Skip to content

Handle apostrophes in fingerprint glob paths#2879

Open
kv1sidisi wants to merge 1 commit into
go-task:mainfrom
kv1sidisi:fix-fingerprint-quote-path
Open

Handle apostrophes in fingerprint glob paths#2879
kv1sidisi wants to merge 1 commit into
go-task:mainfrom
kv1sidisi:fix-fingerprint-quote-path

Conversation

@kv1sidisi

Copy link
Copy Markdown
  • Escape apostrophes when expanding fingerprint globs so quoted paths are parsed correctly.
  • Add a regression test for a directory name that contains an apostrophe.

@trulede trulede left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems OK, I offered some feedback.

"github.com/stretchr/testify/require"
)

func TestGlobHandlesApostropheInPath(t *testing.T) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A table driven test, for each of the cases in the code, would be useful.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this test is not very useful. It would be better to make an Executor/Task test where you have a list of files including characters which should be escaped, then run the Task, then check the files are handled correctly. You might even find that such a test exists.

Such a test would cover the entire processing chain (in case it gets broken somewhere else too).

Comment thread internal/execext/exec.go
Comment on lines 93 to +99
func escape(s string) string {
s = filepath.ToSlash(s)
s = strings.ReplaceAll(s, " ", `\ `)
s = strings.ReplaceAll(s, "&", `\&`)
s = strings.ReplaceAll(s, "(", `\(`)
s = strings.ReplaceAll(s, ")", `\)`)
s = strings.ReplaceAll(s, "'", `\'`)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this would be better:

var pathReplacer = strings.NewReplacer(
	" ", `\ `,
	"&", `\&`,
	"(", `\(`,
	")", `\)`,
	"'", `\'`,
)

func escape(s string) string {
	s = filepath.ToSlash(s)
	return pathReplacer.Replace(s)
}

But I also wonder if there are other characters which need escaping?

@trulede trulede added the area: exec Changes related to the execution of commands. label Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: exec Changes related to the execution of commands.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants